feat: add ResolveDependencies for concrete volume/metadata discovery#135
feat: add ResolveDependencies for concrete volume/metadata discovery#135gfyrag wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 725a05e4bc3cc158af06dfeea38698eaa6676f27 and a1f074b. 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a ChangesDependency Resolution Dry-Run API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aee00c8 to
0b8b90c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
- Coverage 67.81% 67.42% -0.39%
==========================================
Files 49 51 +2
Lines 5223 5388 +165
==========================================
+ Hits 3542 3633 +91
- Misses 1472 1533 +61
- Partials 209 222 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0b8b90c to
52a99af
Compare
Note on
|
|
|
||
| require.Equal(t, deps.Volumes, map[string]map[string]*big.Int{ | ||
| "source1": { | ||
| "COIN": big.NewInt(123), |
There was a problem hiding this comment.
Don't need the amounts, just need to know the touched volumes.
You can keep it if you're comfident with it, but it add you more surface to maintain.
There was a problem hiding this comment.
If we don't need the amount it's better to remove them so that the impl is more "conservative".
However the point here is that this test shows that we still output a "Volumes" result even if the set of involved (account,asset) pairs depends in the meta and/or balances themselves. (In the case of the test, just on the meta). We had talked about this a bit (my understanding is that, if it happens, you'll detect that and just retry the thing) but I'm asking again to double check if we are ok with this
There was a problem hiding this comment.
Yes, it should work. I've not noted the missing metadata key in deps.
But it should work.
Is the resolution determinist? (same calls in the same order?)
There was a problem hiding this comment.
Hum, sorry, wrong response.
The GetAccountMetadata will be exercised in the admission path AND in the hot path.
If the dependency resolution does not report the metadata, it will not be loaded in the raft command, and so, not loaded in the in memory storage of the hot path.
There was a problem hiding this comment.
So, a totally valid transaction will be rejected because the metadata will not be found in ram.
|
@ascandone updated after the rescope discussion — context for the new approach below. RescopeTwo concrete needs:
The hashing itself will be done in What changed vs. the previous commit
Re-flagging on your earlier point — the test now exercises a case where |
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The new API can miss real store reads and some actual balance writes for valid scripts, which breaks the dependency discovery and drift-detection guarantees it is meant to provide.
| } | ||
|
|
||
| writes := map[string]map[string]struct{}{} | ||
| for _, statement := range program.Statements { |
There was a problem hiding this comment.
🔴 [blocker] Include execution-time store reads
For scripts where store-backed functions are evaluated during statement execution rather than source preloading, such as set_tx_meta("k", meta(@cfg, "v")) or caps/allotments using balance(...), Run will call the store but this path only preloads source balances and then walks send statements without executing/evaluating those expressions. In those cases ResolveDependencies omits real metadata/balance reads from the admission hash, so FSM drift can go undetected.
| case *parser.SourceOverdraft: | ||
| return st.touchAccount(source.Address, source.Color, writes) | ||
|
|
||
| case *parser.SourceWithScaling: |
There was a problem hiding this comment.
🟠 [major] Record scaling swap balance writes
When a source uses with scaling through @swap, runtime emits conversion postings from the source to the swap account in scaled asset(s) and from the swap account back to the source before the final send. This records only the source/current asset, so callers relying on Writes.Volumes will miss the swap account and scaled source assets that are actually impacted for asset-scaling scripts.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/recording_store.go`:
- Around line 30-36: The loop iterating over result and accessing
r.balanceReads[account][asset] assumes a nested map structure, but
r.balanceReads[account] is of type BalanceRow, not a map. Verify the actual
return type of Store.GetBalances and the actual structure of BalanceRow, then
update the loop that currently treats r.balanceReads[account] as a map to
instead properly populate the BalanceRow structure according to its real fields
and methods. If needed, convert the result into the appropriate data structure
before storing or adjust how individual balance values are assigned to match the
BalanceRow representation.
In `@internal/interpreter/resolve_dependencies.go`:
- Around line 161-163: In the case statement for *parser.SourceWithScaling, the
code incorrectly accesses source.Color which does not exist on this type.
Examine the actual fields defined on the SourceWithScaling struct to determine
the correct approach. Either access the appropriate fields available on
SourceWithScaling to call touchAccount with the correct arguments, or
recursively resolve the wrapped source node that SourceWithScaling contains (to
extract the actual Address and Color values) before calling touchAccount with
those extracted values.
- Around line 83-89: The CachedBalances field initialization in the programState
struct is using the wrong type. Change the CachedBalances initialization from
Balances{} to InternalBalances{} to match the expected field type of
programState.CachedBalances. Verify this matches the type used in the normal
interpreter state construction path to maintain consistency and avoid type
drift.
- Around line 259-267: The color variable returned from evaluateOptExprAs is a
value type, not a pointer, but the code incorrectly treats it as a pointer with
nil checks and dereference operators. Replace the pointer-based nil and
dereference checks with value-based emptiness checks (checking if color is an
empty string directly). Additionally, the undefined coloredAsset function call
should be replaced with the existing color-application helper function that is
available in the codebase. Update the type handling in this block to match the
actual return type of evaluateOptExprAs and use the correct, defined helper
function for applying color to assets.
- Around line 138-143: The evaluateSentAmt method returns an Asset value type
(not a pointer), but the code attempts to dereference it with the * operator
when assigning to st.CurrentAsset. Remove the pointer dereference operator from
the assignment statement where st.CurrentAsset = *asset should be changed to
st.CurrentAsset = asset, since asset is already a value type and does not need
to be dereferenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0d9a3f3-16de-45e2-8879-9204cfb33abf
📒 Files selected for processing (4)
internal/interpreter/recording_store.gointernal/interpreter/resolve_dependencies.gointernal/interpreter/resolve_dependencies_test.gonumscript.go
Add ParseResult.ResolveDependencies() which resolves all balance and metadata reads a script depends on, returning concrete values rather than expression trees. This enables consumers (e.g. ledger v3) to: - Preload exact volumes needed before FSM execution - Compute an input hash for optimistic concurrency control - Detect drift between admission and execution time Unlike GetInvolvedAccounts (which returns expression trees requiring consumer-side resolution), ResolveDependencies returns concrete (account, asset) → balance and (account, key) → value maps. The consumer doesn't need to walk expression trees or handle deferred resolution. All features are supported transparently: meta() chains, balance() in account addresses, mid-script function calls, oneof, etc. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Walk the source/destination AST to collect touched (account, asset) pairs instead of fully running the script. Reads still come from phases 1-2 (parseVars + balance preloading), Writes are now a conservative over-approximation of every account mentioned as a source or destination. Also restructure the output as Reads / Writes and drop ForbiddenFlags (no longer needed now that we don't execute statements).
725a05e to
a1f074b
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
ResolveDependencies can omit concrete store reads that are evaluated by Run and can under-report writes for asset scaling. These gaps break the intended dependency discovery and drift/conflict detection semantics.
| } | ||
| st.varOriginPosition = false | ||
|
|
||
| for _, statement := range program.Statements { |
There was a problem hiding this comment.
🔴 [blocker] Include reads from function and cap expressions
For scripts where a store-reading expression appears outside variable declarations, sent values, or account address expressions — for example set_tx_meta("x", meta(@cfg, "k")) or a source/destination cap using balance(@limits, USD/2) — Run will evaluate those expressions and read the store, but this resolver only performs the balance preload pass here and never evaluates those statement/cap/allotment expressions. The returned Reads can therefore omit inputs that affect execution, so the admission/FSM drift hash can miss metadata or balance changes.
| case *parser.SourceOverdraft: | ||
| return st.touchAccount(source.Address, source.Color, writes) | ||
|
|
||
| case *parser.SourceWithScaling: |
There was a problem hiding this comment.
🟠 [major] Record scaling writes for swap and scaled assets
When the script uses source = @src with scaling through @swap, the interpreter can emit postings from src in the consumed scaled asset rows and from swap in the target asset before the final send. This branch records only src with CurrentAsset, so Writes.Volumes misses balances such as src/EUR and swap/EUR/2; callers using writes for preloading or conflict detection will not protect balances the script can update.
Context
Ledger v3 uses a Raft architecture where transactions go through two stages:
The admission layer needs to know which volumes to preload before executing the script — but the volumes depend on the script itself.
The current approach uses
GetInvolvedAccountswhich returns expression trees (InvolvedAccountExpr) that the consumer must resolve. This led to ~500 lines of expression tree walking code on the ledger side, with bugs (GetAmount/GetAsset extracting wrong component, IsValidCall never called) and limitations (balance-dependent account addresses rejected).Proposal
Add
ResolveDependencies()which returns concrete data instead of expression trees:The consumer provides a
Storecallback (same interface asRun), gets back concrete reads, and handles the rest (hashing, preloading, drift detection) on its side.How the ledger uses it
Admission: call
ResolveDependencies→ get volumes + metadata → hash inputs → preload into Raft orderFSM: call
ResolveDependenciesagain → hash inputs → compare with admission hash → if match, execute; if mismatch, reject (retryable)This is classic optimistic concurrency control.
What this replaces on the ledger side
resolveExprToString,resolveExprWithMetadata,collectFnMetaReads,ResolveDeferredAccountsInvolvedAccountExprtypesIsValidCall/ non-deterministic script rejectionImplementation
recordingStorewraps aStoreand records allGetBalances/GetAccountsMetadatacallsResolveDependenciesruns the script with the recording store, discards the execution result, returns the recorded readsTest plan
🤖 Generated with Claude Code